<html>
<head><meta charset="utf-8"><title>need review · t-compiler/rust-analyzer · Zulip Chat Archive</title></head>
<h2>Stream: <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/index.html">t-compiler/rust-analyzer</a></h2>
<h3>Topic: <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html">need review</a></h3>

<hr>

<base href="https://rust-lang.zulipchat.com">

<head><link href="https://rust-lang.github.io/zulip_archive/style.css" rel="stylesheet"></head>

<a name="194767756"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194767756" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194767756">(Apr 21 2020 at 07:17)</a>:</h4>
<p>Hello <span class="user-mention" data-user-id="133169">@matklad</span>  this PR needs you <a href="https://github.com/rust-analyzer/rust-analyzer/pull/3954" title="https://github.com/rust-analyzer/rust-analyzer/pull/3954">https://github.com/rust-analyzer/rust-analyzer/pull/3954</a> 🥰 can you provide me some information or your thoughts about my proposal ? I had also another idea, maybe I can not disable sortText when there is some characters after the dot for autocompletion. It will limit risks. What do you think ?</p>



<a name="194817369"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194817369" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194817369">(Apr 21 2020 at 15:16)</a>:</h4>
<p>Thanks for the review. Can you let me know which tool do you use to make your gif demo of new features in rust-analyzer ? It's awesome</p>



<a name="194817560"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194817560" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> matklad <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194817560">(Apr 21 2020 at 15:18)</a>:</h4>
<p>peek</p>



<a name="194817681"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194817681" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> matklad <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194817681">(Apr 21 2020 at 15:18)</a>:</h4>
<p>(sorry for the long review roundtrips and pretty short reviews. I've been swamped as of lately. Probably will be for the forseeable future as well :) )</p>



<a name="194817976"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194817976" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194817976">(Apr 21 2020 at 15:20)</a>:</h4>
<p>No worry ! Your work is incredible and I know you're very busy ! Thanks for all :)</p>



<a name="194821376"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194821376" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194821376">(Apr 21 2020 at 15:43)</a>:</h4>
<p><a href="/user_uploads/4715/p7ArrN2s1v22eAVQ5lFuyfJe/ra_autocomplete.gif" title="ra_autocomplete.gif">ra_autocomplete.gif</a> Here is the behavior with a simple example</p>
<div class="message_inline_image"><a href="/user_uploads/4715/p7ArrN2s1v22eAVQ5lFuyfJe/ra_autocomplete.gif" title="ra_autocomplete.gif"><img src="/user_uploads/4715/p7ArrN2s1v22eAVQ5lFuyfJe/ra_autocomplete.gif"></a></div>



<a name="194826463"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194826463" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> std::Veetaha <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194826463">(Apr 21 2020 at 16:19)</a>:</h4>
<p>Looks awesome, but I'd like to suggest to have a feature toggle for this one (if it doesn't already exit).<br>
The tool is screencast mode:<br>
<a href="/user_uploads/4715/3pCWfTDCkYoYyV_z4FFZqm_n/image.png" title="image.png">image.png</a></p>
<div class="message_inline_image"><a href="/user_uploads/4715/3pCWfTDCkYoYyV_z4FFZqm_n/image.png" title="image.png"><img src="/user_uploads/4715/3pCWfTDCkYoYyV_z4FFZqm_n/image.png"></a></div>



<a name="194903787"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194903787" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194903787">(Apr 22 2020 at 08:58)</a>:</h4>
<p>About this PR <a href="https://github.com/rust-analyzer/rust-analyzer/pull/4043" title="https://github.com/rust-analyzer/rust-analyzer/pull/4043">https://github.com/rust-analyzer/rust-analyzer/pull/4043</a> is anyone to help me find a way to fetch tail expressions easily ? Or redirect me to an example where it's already used ?</p>



<a name="194914502"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194914502" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Florian Diebold <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194914502">(Apr 22 2020 at 10:57)</a>:</h4>
<p>I think right now we have no easy way of finding all tail expressions, so you probably have to walk through the syntax tree and handle it based on the syntax nodes. The <code>match_ast</code> macro could come in handy there though.</p>



<a name="194915482"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194915482" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Florian Diebold <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194915482">(Apr 22 2020 at 11:10)</a>:</h4>
<p>I imagine we might need something like it for some analyses later, but we don't have them yet</p>



<a name="194915889"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194915889" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194915889">(Apr 22 2020 at 11:16)</a>:</h4>
<p>Ok thank you. I started to write a function to fetch it. Not so easy but at least I know that I'm not rewriting this instead of using an existing function.</p>



<a name="194920795"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194920795" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Florian Diebold <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194920795">(Apr 22 2020 at 12:13)</a>:</h4>
<p>although I wonder how much the assist should really go into expressions -- it's not clear whether turning <code>if x { foo } else { bar }</code> into <code>if x { Ok(foo) } else { Ok(bar) }</code> or <code>Ok(if x { foo } else { bar })</code> is better <span aria-label="thinking" class="emoji emoji-1f914" role="img" title="thinking">:thinking:</span> </p>
<p>apart from that, finding <code>return</code> expressions is important though</p>



<a name="194921469"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194921469" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Laurențiu <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194921469">(Apr 22 2020 at 12:20)</a>:</h4>
<p>But not <code>return</code> expressions in closures or inner functions. Also,</p>
<div class="codehilite"><pre><span></span><span class="k">fn</span> <span class="nf">foo</span><span class="p">()</span><span class="w"> </span>-&gt; <span class="kt">i32</span> <span class="p">{</span><span class="w"></span>
<span class="w">    </span><span class="k">loop</span><span class="w"> </span><span class="p">{</span><span class="w"></span>
<span class="w">        </span><span class="k">break</span><span class="w"> </span><span class="mi">42</span><span class="p">;</span><span class="w"></span>
<span class="w">    </span><span class="p">}</span><span class="w"></span>
<span class="p">}</span><span class="w"></span>
</pre></div>



<a name="194929945"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194929945" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194929945">(Apr 22 2020 at 13:29)</a>:</h4>
<p>Yes already thought about that. For now I think it's better to go deeper instead of just returing <code>Ok(if...)</code>. I'm not so far. I have some issues with pattern with a tail expression like a match for example and inside this match a return expression lol</p>



<a name="194930039"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194930039" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194930039">(Apr 22 2020 at 13:30)</a>:</h4>
<p>example: </p>
<div class="codehilite"><pre><span></span>fn foo() -&gt; i32 {
                let my_var = 5;
                match my_var {
                    5 =&gt; {
                        if true {
                            42i32
                        } else {
                            25i32
                        }
                    },
                    _ =&gt; {
                        let test = &quot;test&quot;;
                        if test == &quot;test&quot; {
                            return 24i32;
                        }
                        53i32
                    },
                }
            }
</pre></div>



<a name="194931721"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194931721" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Timo Freiberg <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194931721">(Apr 22 2020 at 13:41)</a>:</h4>
<p>Not sure if this applies to this case, but I think when there are many valid results of an assist and the decision is a question of style, it makes sense to implement the easiest version and add another assist for converting between the different versions (example: generating qualified paths or adding a <code>use</code> statement &amp; generating unqualified paths)</p>



<a name="194936757"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194936757" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Florian Diebold <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194936757">(Apr 22 2020 at 14:14)</a>:</h4>
<p>I think we did talk about having assists that move the <code>Ok</code> around as well, yeah</p>



<a name="194955729"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194955729" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194955729">(Apr 22 2020 at 16:20)</a>:</h4>
<p><a href="/user_uploads/4715/auEN0ijqr9NGKgajIwKgaMFA/ok_wrapper.gif" title="ok_wrapper.gif">ok_wrapper.gif</a> I updated the PR, here is an example :)</p>
<div class="message_inline_image"><a href="/user_uploads/4715/auEN0ijqr9NGKgajIwKgaMFA/ok_wrapper.gif" title="ok_wrapper.gif"><img src="/user_uploads/4715/auEN0ijqr9NGKgajIwKgaMFA/ok_wrapper.gif"></a></div>



<a name="194958809"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194958809" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Jeremy Kolb <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194958809">(Apr 22 2020 at 16:46)</a>:</h4>
<p>That looks nice!</p>



<a name="194959041"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194959041" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Jeremy Kolb <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194959041">(Apr 22 2020 at 16:47)</a>:</h4>
<p>bike shed: I think the label should be similar to "Wrap with Ok" or "Wrap return value with Ok" instead of "Make..."</p>



<a name="194960335"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194960335" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> std::Veetaha <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194960335">(Apr 22 2020 at 16:57)</a>:</h4>
<p>I'd suggest "Change return type to Result"</p>



<a name="194963884"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194963884" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Jeremy Kolb <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194963884">(Apr 22 2020 at 17:21)</a>:</h4>
<p>good point</p>



<a name="194964931"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/194964931" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#194964931">(Apr 22 2020 at 17:29)</a>:</h4>
<p>Yes for the function name and the label I was counting on you to make proposals haha</p>



<a name="195107838"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/195107838" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#195107838">(Apr 23 2020 at 19:24)</a>:</h4>
<p>If you have time to take a look, this one <a href="https://github.com/rust-analyzer/rust-analyzer/pull/3954" title="https://github.com/rust-analyzer/rust-analyzer/pull/3954">https://github.com/rust-analyzer/rust-analyzer/pull/3954</a> is ready to review.<br>
And also this one <a href="https://github.com/rust-analyzer/rust-analyzer/pull/4043" title="https://github.com/rust-analyzer/rust-analyzer/pull/4043">https://github.com/rust-analyzer/rust-analyzer/pull/4043</a></p>



<a name="195428706"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/185405-t-compiler/rust-analyzer/topic/need%20review/near/195428706" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Coenen Benjamin <a href="https://rust-lang.github.io/zulip_archive/stream/185405-t-compiler/rust-analyzer/topic/need.20review.html#195428706">(Apr 27 2020 at 14:53)</a>:</h4>
<p>Hello <span class="user-mention" data-user-id="133169">@matklad</span>  let me know how can I help you to make the task easier for you to review this kind of PR <a href="https://github.com/rust-analyzer/rust-analyzer/pull/4043" title="https://github.com/rust-analyzer/rust-analyzer/pull/4043">https://github.com/rust-analyzer/rust-analyzer/pull/4043</a> ? Maybe I can add a better explanation about the implementation ? Let me know :)</p>



<hr><p>Last updated: Aug 07 2021 at 22:04 UTC</p>
</html>